Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add skipLibCheck, noEmit, consistentCasing, and lots of comments to template tsconfigs #864

Merged
merged 4 commits into from
Sep 17, 2020

Conversation

agilgur5
Copy link
Collaborator

Description

Commits

feat: add skipLibCheck and forceConsistentCasingInFileNames

  • as defaults to the templates' tsconfigs

  • skipLibCheck is a big perf gain, though it seems to only affect
    tsc --noEmit usage and rollup-plugin-typescript2 seems to ignore it

    • I have an existing issue open on the matter
  • forceConsistentCasingInFileNames is a no-brainer to help avoid a bug
    that is easy to make and hard to debug

  • also add a trailing comma for ease of use and better diffing

    • not to fixtures though, because there is a test for this

feat: add noEmit to templates' tsconfigs

  • for ease of use when using tsc for type-checking, which a lot of
    users already do

    • it doesn't fully type-check everything though, since we'd need to
      split off a tsconfig.build.json in order to let builds only run on
      src while type-checking would run on all files
      • but people already using tsc --noEmit, so might as well start
        supporting this as it's not uncommon
  • also add to all fixtures

    • yea some centralization/extends or dogfooding create would help
      reduce all this duplication

docs: add comments for nearly all tsconfig options in use

  • since we can use comments in tsconfig.json, add them to give simple,
    one-liner explanations as to what each setting does

  • also add a link to the TSConfig Reference for further details

  • and specify which options are recommended as true by TS but not on by
    default for backwards compat or other reasons

  • a few leftover that I still need to think about

docs: add comment that noUnused* overlaps with an ESLint rule

  • the checks overlap with the @typescript-eslint/no-unused-vars rule

  • per user issue, this can result in duplicative errors inside an IDE,
    so explicitly call this out in a comment for tsdx lint / ESLint
    users in case they'd like to disable this and leave only one on

  • change the ordering of linter checks in the templates as well to make
    the comments easier to read/understand

Tags

Review Notes

Key thing to check would be that all 3 templates and all 6 fixtures are similarly updated

- as defaults to the templates' tsconfigs
  - these are now recommended by TS to be set to true, which can be
    seen in the TSConfig Reference and @tsconfig/recommended
    - c.f. https://www.typescriptlang.org/tsconfig and
      https://www.npmjs.com/package/@tsconfig/recommended
  - also add to all fixtures
    - yea some centralization/extends or dogfooding create would help
      reduce all this duplication

- skipLibCheck is a big perf gain, though it seems to only affect
  `tsc --noEmit` usage and rollup-plugin-typescript2 seems to ignore it
  - I have an existing issue open on the matter
- forceConsistentCasingInFileNames is a no-brainer to help avoid a bug
  that is easy to make and hard to debug

- also add a trailing comma for ease of use and better diffing
  - not to fixtures though, because there is a test for this
- for ease of use when using `tsc` for type-checking, which a lot of
  users already do
  - it doesn't fully type-check everything though, since we'd need to
    split off a tsconfig.build.json in order to let builds only run on
    `src` while type-checking would run on all files
    - but people already using `tsc --noEmit`, so might as well start
      supporting this as it's not uncommon

- also add to all fixtures
  - yea some centralization/extends or dogfooding create would help
    reduce all this duplication
- since we can use comments in tsconfig.json, add them to give simple,
  one-liner explanations as to what each setting does
- also add a link to the TSConfig Reference for further details
- and specify which options are recommended as true by TS but not on by
  default for backwards compat or other reasons

- a few leftover that I still need to think about
- the checks overlap with the @typescript-eslint/no-unused-vars rule
- per user issue, this can result in duplicative errors inside an IDE,
  so explicitly call this out in a comment for `tsdx lint` / ESLint
  users in case they'd like to disable this and leave only one on

- change the ordering of linter checks in the templates as well to make
  the comments easier to read/understand
@vercel

This comment has been minimized.

@agilgur5 agilgur5 added the scope: templates Related to an init template, not necessarily to core (but could influence core) label Sep 17, 2020
Copy link
Collaborator Author

@agilgur5 agilgur5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍 Diff was fairly straightforward to go through due to spacing between comments making it easier on the eyes

@agilgur5
Copy link
Collaborator Author

Failing CI check is another one of those GH Actions macOS timeouts, not an actual test failure. Should be good to go!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope: templates Related to an init template, not necessarily to core (but could influence core)
Projects
None yet
1 participant